Skip to content

Conversation

@PeraSite
Copy link
Member

@PeraSite PeraSite commented Sep 23, 2025

Summary

서버가 꺼지거나 응답할 수 없는 경우와 네트워크 연결이 아예 기기에서 안될 때 발생하던 UnknownHostException, SocketTimeoutException 대응 추가
okhttp3로 보내는 모든 서버 요청에서 IOException가 발생하면 AlertDialog를 띄우는 ErrorActivity로 이동합니다.

Describe your changes

image

Issue

To reviewers

  • Intro 에서만 서버 통신 오류 확인하지 않고 모든 네트워크 요청의 오류에 대응하도록 작성했습니다. 앱을 정상적으로 사용하고 있다가 서버가 죽었으면 그때도 오류 페이지로 넘어가야할 것 같더라구요!
  • 다만 기존 IntroViewModel처럼 오류 발생 시 LoginActivity와 같이 새로운 Activity로 이동하는 경우, NetworkErrorInterceptor가 ErrorActivity로 이동시킨 이후에 새로운 Activity로 이동하게 됩니다.
  • 그래서 새 ViewModel을 만들 떄 네트워크 요청 > 오류 발생 시 Activity 이동과 같은 흐름에서 문제가 없는지 검토해야 해요. 지금 코드에서는 문제 없습니다!

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @PeraSite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

이 PR은 애플리케이션의 네트워크 오류 처리 메커니즘을 개선하여 견고성을 높이는 것을 목표로 합니다. 서버 연결 불가 또는 응답 없음(예: UnknownHostException, SocketTimeoutException)과 같은 상황에서 사용자에게 일관된 경험을 제공하기 위해, 모든 네트워크 요청에 대해 오류 발생 시 전용 ErrorActivity로 이동하여 사용자에게 오류 메시지를 표시하도록 구현되었습니다. 이는 특정 초기 검사뿐만 아니라 앱 전반의 네트워크 실패에 대한 포괄적인 대응을 가능하게 합니다.

Highlights

  • 네트워크 오류 처리: 네트워크 오류 발생 시 사용자에게 일관된 메시지를 제공하기 위한 ErrorActivity를 추가했습니다.
  • NetworkErrorInterceptor 구현: IOException (예: UnknownHostException, SocketTimeoutException)을 감지하고 처리하는 NetworkErrorInterceptor를 구현했습니다.
  • OkHttpClient 통합: 인증된/인증되지 않은 OkHttpClient 설정에 NetworkErrorInterceptor를 통합하여 모든 네트워크 요청에 대한 오류 처리를 중앙화했습니다.
  • IntroViewModel 로직 수정: IntroViewModel에서 500번대 오류 발생 시
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

이번 PR은 OkHttp Interceptor를 사용하여 전역 네트워크 오류 처리기를 도입하여 오류 처리를 중앙 집중화하는 좋은 접근 방식을 보여줍니다. IOException이 발생하면 ErrorActivity로 이동하여 사용자에게 알립니다. 제 리뷰에는 구현을 개선하기 위한 몇 가지 제안이 포함되어 있습니다:

  • OkHttpClient provider를 리팩토링하여 코드 중복을 줄입니다.
  • ErrorActivity를 시작할 때 인텐트 플래그를 조정하여 사용자 경험이 저해되는 것을 방지합니다.
  • ErrorActivity의 뷰 바인딩을 수정합니다.
  • 하드코딩된 값을 제거하고 중복 코드를 정리하여 코드 유지보수성을 향상시킵니다.
    전반적으로 올바른 방향으로 변경되었으며, 몇 가지 조정을 거치면 오류 처리 메커니즘이 더욱 견고하고 사용자 친화적으로 될 것입니다.

return chain.proceed(request)
} catch (e: IOException) {
val intent = Intent(context, ErrorActivity::class.java).apply {
addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Intent.FLAG_ACTIVITY_CLEAR_TASK 플래그는 현재 액티비티 스택을 모두 지우고 새로운 태스크를 시작합니다. 일시적인 네트워크 오류(예: 터널 통과)가 발생했을 때 사용자가 보고 있던 화면이 모두 사라지고 오류 화면만 남게 되어 사용자 경험을 크게 해칠 수 있습니다. 이 플래그를 제거하여 사용자가 오류 확인 후 이전 화면으로 돌아갈 수 있도록 하는 것이 좋습니다. FLAG_ACTIVITY_NEW_TASK만으로도 비-액티비티 컨텍스트에서 액티비티를 시작하는 데 충분합니다.

Suggested change
addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK)
addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)

@AndroidEntryPoint
class ErrorActivity : AppCompatActivity() {

private lateinit var binding: ActivityIntroBinding
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

ErrorActivity에서 ActivityIntroBinding을 사용하고 있습니다. 이는 ErrorActivity가 인트로 화면의 레이아웃을 사용하게 만들어, 의도치 않은 UI가 표시될 수 있습니다. ErrorActivity를 위한 별도의 레이아웃 파일(예: activity_error.xml)과 그에 맞는 뷰 바인딩 클래스(예: ActivityErrorBinding)를 생성하여 사용하는 것이 올바릅니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

대응햇슨

Comment on lines 59 to 79
fun provideAuthOkHttpClient(
tokenInterceptor: TokenInterceptor,
tokenAuthenticator: TokenAuthenticator
tokenAuthenticator: TokenAuthenticator,
networkErrorInterceptor: NetworkErrorInterceptor
) = if (BuildConfig.DEBUG) {
val loggingInterceptor = HttpLoggingInterceptor()
loggingInterceptor.setLevel(HttpLoggingInterceptor.Level.BODY)

OkHttpClient.Builder()
.addInterceptor(networkErrorInterceptor)
.addInterceptor(loggingInterceptor)
.addInterceptor(tokenInterceptor)
.authenticator(tokenAuthenticator)
.build()
} else {
// 프로덕션 환경에서는 로깅 인터셉터를 추가하지 않음
OkHttpClient.Builder()
.addInterceptor(networkErrorInterceptor)
.addInterceptor(tokenInterceptor)
.authenticator(tokenAuthenticator)
.build()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

현재 provideAuthOkHttpClient 함수는 BuildConfig.DEBUG 값에 따라 두 개의 거의 동일한 OkHttpClient.Builder 블록을 가지고 있습니다. 로깅 인터셉터 추가 여부만 다른데, 이로 인해 코드 중복이 발생합니다. 아래와 같이 리팩토링하여 중복을 제거하고 가독성을 높일 수 있습니다.

    fun provideAuthOkHttpClient(
        tokenInterceptor: TokenInterceptor,
        tokenAuthenticator: TokenAuthenticator,
        networkErrorInterceptor: NetworkErrorInterceptor
    ): OkHttpClient {
        val builder = OkHttpClient.Builder()
            .addInterceptor(networkErrorInterceptor)
            .addInterceptor(tokenInterceptor)
            .authenticator(tokenAuthenticator)

        if (BuildConfig.DEBUG) {
            val loggingInterceptor = HttpLoggingInterceptor().apply {
                level = HttpLoggingInterceptor.Level.BODY
            }
            builder.addInterceptor(loggingInterceptor)
        }

        return builder.build()
    }

} catch (e: IOException) {
val intent = Intent(context, ErrorActivity::class.java).apply {
addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK)
putExtra("message", "서버 통신에 실패했습니다. 잠시 후 다시 시도해 주세요.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

에러 메시지와 인텐트 extra의 키("message")가 하드코딩되어 있습니다. 유지보수성과 다국어 지원을 위해 문자열은 strings.xml 리소스에 정의하고, 인텐트 키는 ErrorActivity의 companion object에 상수로 정의하여 사용하는 것을 권장합니다.

예시:

// ErrorActivity.kt
companion object {
    const val EXTRA_MESSAGE = "error_message"
}

// strings.xml
<string name="network_error">서버 통신에 실패했습니다. 잠시 후 다시 시도해 주세요.</string>

// NetworkErrorInterceptor.kt
putExtra(ErrorActivity.EXTRA_MESSAGE, context.getString(R.string.network_error))


val baseResponse = BaseResponse<Void>(
isSuccess = false,
code = 500, // 서버 처리 오류인지 통신 불가인지 구분
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

네트워크 오류를 나타내기 위해 code500으로 하드코딩하고 있습니다. 이 값은 IntroViewModel에서도 사용되므로, 두 파일 간의 의존성이 생깁니다. 이 값을 NetworkErrorInterceptorcompanion object에 상수로 정의하여 여러 곳에서 참조하도록 하면 코드의 일관성과 유지보수성을 높일 수 있습니다.

예시:

// NetworkErrorInterceptor.kt
companion object {
    const val NETWORK_ERROR_CODE = 500
    private val gson = Gson()
}

// IntroViewModel.kt
... else if (it.code != NetworkErrorInterceptor.NETWORK_ERROR_CODE) { ...

if (it.result == true) { //토큰이 있고 유효함
_uiState.value = UiState.Success(IntroState.ValidToken)
} else { //토큰이 있어도 유효하지 않음
} else if (it.code != 500) { // 토큰이 있어도 유효하지 않음
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

매직 넘버 500을 사용하여 네트워크 오류 케이스를 확인하고 있습니다. 이 값은 NetworkErrorInterceptor에서 설정한 값과 동일해야 합니다. 이러한 값은 한 곳(예: NetworkErrorInterceptorcompanion object)에 상수로 정의하고 여러 곳에서 참조하도록 하여, 코드의 가독성을 높이고 잠재적인 버그를 방지하는 것이 좋습니다.

Copy link
Member

@HI-JIN2 HI-JIN2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일단 서버가 health한지는 502 gateway를 경계해야합니다.
500은 클라이언트가 보낸 요청을 서버에서 처리할 수 없을때 반환되며,
서버 자체가 내려갔을때는 502가 반환됩니다.
스크린샷 2025-09-30 오후 11 49 22

현재 dev 서버 점검 중으로 브랜치 들어가서 테스트를 해봤는데 (+500 ->502로 바꾼채 테스트)

토큰이 있는 상황) 인트로 -> 로그인으로 넘어감 -> 로그인 시도하면 "로그인에 실패했습니다" 토스트가 뜨는데, 이 플로우가 의도하신 플로우가 맞을까요? 설정하신 다이알로그가 뜨지 않아서요..!

@PeraSite
Copy link
Member Author

PeraSite commented Sep 30, 2025

일단 서버가 health한지는 502 gateway를 경계해야합니다. 500은 클라이언트가 보낸 요청을 서버에서 처리할 수 없을때 반환되며, 서버 자체가 내려갔을때는 502가 반환됩니다. 스크린샷 2025-09-30 오후 11 49 22

현재 dev 서버 점검 중으로 브랜치 들어가서 테스트를 해봤는데 (+500 ->502로 바꾼채 테스트)

토큰이 있는 상황) 인트로 -> 로그인으로 넘어감 -> 로그인 시도하면 "로그인에 실패했습니다" 토스트가 뜨는데, 이 플로우가 의도하신 플로우가 맞을까요? 설정하신 다이알로그가 뜨지 않아서요..!

정확히 구분해보자면 이 PR에서 제가 해결한 부분은 서버와 소켓 연결 자체를 실패했을 때 발생하는 오류를 대응하는겁니다!
그래서 NetworkErrorInterceptor에서도 SocketTimeoutException와 UnknownHostException 등 소켓 오류의 부모인 java.IOException을 처리하고 있어요.

말씀해주신 상황은 서버와 소켓 연결은 성공했는데, 리버스 프록시나 잇슈 백엔드에서 5xx Response를 던져줬을 때입니다. 그런 경우에는 Retrofit에서 자체적으로 5xx라면 무조건 오류를 내게끔 설계가 되어있어요. 캡처해주신 사진은 그 오류입니다!
따라서 Retrofit이 발생시키는 retrofit.HttpException 에 대응이 필요하다고 생각합니다.

okhttp3 처리 > Retrofit 처리 순으로 네트워크 데이터 흐름이 진행되는데, okhttp3 단계의 NetworkErrorInterceptor가 실제 네트워크 응답을 5xx대로 설정하면 Retrofit에서 오류를 내니
실제 네트워크 응답은 200으로 하되, 잇슈 DTO 상 JSON Response Body에 있는 code 라는 필드 값을 임의로 500으로 설정해 비즈니스 로직에서 검사하도록 작성했어요.

아무래도 이 부분은 필히 리팩토링이 필요하겠으나... 당장 비정상 종료율을 없애야할 것 같아 먼저 PR 올렸습니다!

@HI-JIN2
Copy link
Member

HI-JIN2 commented Sep 30, 2025

아하 제가 의도를 잘못이해하였었군요..! IOException(SocketTimeoutException, UnknownHostException)에 대한 에러 대응으로 다시 이해하고 이야기를 해볼게요.

제 짧은 견해로 보자면 일단 네트워크 오류에 전사적으로 대응하는 일반적인 방법은 ApiResult sealed class를 만드는 것입니다. 이부분에 대해서 @kangyuri1114 님과 이야기를 나누었을떄, 요러한 방법으로 진행하면 좋겠다고 했었고, 제가 아는 보편적인 방법도 해당 방법입니다.

interceptor를 오버라이딩 해보셔서 알겠지만 interceptor의 역할은 api call을 요청하기 전에 낚아채서 뭔가 덧붙인 다음에 요청을 보내도록 하는 것입니다. 소프트웨어 엔지니어링에 정답은 없지만, 저는 제훈님께서 구현한 방식이 interceptor 답지 못하다고 억지스러운 구현이라는 생각이 듭니다..!

더불어 네트워크 레이어인 interceptor에서 UI를 직접적으로 호출하는것은 바람직하지 않은 것 같습니다!

https://itstory1592.tistory.com/132의 맨 마지막 ApiResult을 보시면 도움이 될 것 같습니다

혹, 제가 잘못알고 있는 부분이 있다면 말씀해주시면 감사하겠습니다!

sealed class ApiResult<out T : Any> {
    fun <R : Any> map(transform: (T) -> R): ApiResult<R> = when (this) {
        is Success -> Success(transform(data))
        is Failure -> Failure(responseCode, message)
        is NetworkError -> NetworkError
        is Unexpected -> Unexpected(error)
    }
}

data class Success<T : Any>(val data: T) : ApiResult<T>()
data class Failure(val responseCode: Int, val message: String?) : ApiResult<Nothing>()
object NetworkError : ApiResult<Nothing>()
data class Unexpected(val error: Throwable?) : ApiResult<Nothing>()

@PeraSite
Copy link
Member Author

PeraSite commented Oct 1, 2025

저도 인터셉터를 작성하면서 아.. 이건 아닌데... 싶으면서도 처음 접해보는 라이브러리다보니 넘겼는데 정말 좋은 대안이 있네요! 말씀해주신 것처럼 UI 호출도 코드 스멜이 확실히 있구요!
ApiResult 형태로 매핑하는 것이 가장 우아해보입니다만 아무래도 모든 API 호출을 다 리팩토링해야하니 꽤 큰 규모의 수정이 될 것 같아요.
이 PR을 닫고 그 방향으로 진행하는 것이 좋을까요? @HI-JIN2 @kangyuri1114

@kangyuri1114
Copy link
Member

확인이 늦었네요..!
지금도 서버가 종종 죽고 있어서 빠르게 해결하고 싶은 부분이라 고민이 되긴하네요..
진님 말씀대로 이미 전부터 ApiResult sealed class 도입을 고려하고 있었습니다!

작성해주신 코드는 일시적인 해결에는 좋을 것 같지만, 가장 걸리는 부분은 "인터셉터에서의 intent"입니다

그리고 TimeOut, 서버가 꺼지거나 응답할 수 없는 경우, 네트워크 연결이 아예 기기에서 안될 때 발생하던 UnknownHostException, SocketTimeoutException 등등의 이 모든 예외를 "500대 서버 오류"로 한번에 보는 게 과연 적절한 예외처리일지 궁금합니다

UnknownHostException이나 SocketTimeoutException은 사실 HTTP 요청<>응답이 오가기 전에 발생하는 예외이기에 네트워크 예외에 속하기 때문에 "서버 오류"는 아닌 것 같아서요! 물론 서버가 죽으면 timeout이 나긴 하겠지만, 다른 500대의 진짜 서버 오류와 혼동될 것 같아 우려돼요

-> 여기까지는 코드 리뷰였구요! 그런데 이런 제 우려들이 위의 진님의 예시코드에서 다 해결이 될 것 같네요!

많이 고민되기 하는데.. 오래 걸리더라도 ApiResult sealed class을 도입하는 게 좋을 거 같아요. PR은 닫고 새로 작업해주심이 좋을 것 같습니다~!

@PeraSite
Copy link
Member Author

PeraSite commented Oct 1, 2025

진님 유리님 의견 감사해요! 최대한 빠르게 ApiResult PR 만들어서 올려보겠습니당 👍

@PeraSite PeraSite closed this Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

앱 진입 직전에 서버가 health한지 체크하기 500대 에러 대응 페이지 추가

4 participants